Fix five correctness bugs in the binding engine + add regression tests - #8
Merged
Conversation
Phase 1 of the review fix plan: self-contained, non-breaking correctness
fixes, each verified against the suite (88 tests green on net8.0 + net10.0).
- Register EnumTypeConverter so enum properties bind from strings instead of
throwing InvalidCastException. The converter existed but was never added to
the converter chain, so enums fell through to Convert.ChangeType.
- Parse scalar values with InvariantCulture in DefaultTypeConverter. Values
arrive as strings, so double/decimal previously parsed under the ambient
culture (e.g. "1.5" bound to 15 on a de-DE host).
- BindingContext.PropertyType now returns the property's type instead of its
declaring interface (it was set to propertyInfo.DeclaringType). Also drop two
dead, always-false null checks in the ctor.
- SettingsOptionsValidator no longer throws when AttributeType is null while
interface/suffix indication is set; the Attribute check is now guarded.
- Fix two broken error-message templates in Resources (a missing $ that emitted
a literal {typeName}, and a stray $ before {type.FullName}).
Tests: add Conversion/DefaultTypeConverterTests (culture-invariant parsing) and
Conversion/EnumConversionTests (enum-from-string + attribute default). Both were
written to fail against the pre-fix code, then confirmed green after the fixes.
guy-lud
added a commit
that referenced
this pull request
Jul 12, 2026
Guards the B4 fix (PR #8): BindingContext.PropertyType must expose each property's own type, not its declaring interface. Verified fail-first — with PropertyType = propertyInfo.DeclaringType the assertion reports the property type as the settings interface (ICaptureSettings) instead of System.String. A second test documents that the fix did not remove access to the declaring type: it stays reachable via SettingsType and PropertyInfo.DeclaringType.
guy-lud
added a commit
that referenced
this pull request
Jul 12, 2026
Guards the B4 fix (PR #8): BindingContext.PropertyType must expose each property's own type, not its declaring interface. Verified fail-first — with PropertyType = propertyInfo.DeclaringType the assertion reports the property type as the settings interface (ICaptureSettings) instead of System.String. A second test documents that the fix did not remove access to the declaring type: it stays reachable via SettingsType and PropertyInfo.DeclaringType.
guy-lud
added a commit
that referenced
this pull request
Jul 12, 2026
Docs only; no code changes. - FIX-PLAN.md (new): the prioritized, per-item fix plan from the three-part code review (architecture, tests, performance), with a progress banner. - SESSION-HANDOFF.md: replace the stale modernization-era handoff with a current one covering the review, the fixes shipped this cycle (PRs #8/#10/#11/#12), key decisions (Validations and EqualityCompererCreator held for upcoming feature work), ranked next steps, and gotchas. The push/PR recipe is kept in private notes rather than inlined.
guy-lud
added a commit
that referenced
this pull request
Jul 12, 2026
* Perf quick wins: enumerator, suffix match, env binder, type cache (Q1–Q4) Five FIX-PLAN quick wins; Q5 was already resolved by B4 (#8), so this is Q1–Q4. - Q1 SettingsCollection.GetEnumerator: was rebuilding a whole Dictionary via ToDictionary on every enumeration; now yields over the backing dictionary. - Q2 SettingsTypesExtractor: replace Name.ToLower().EndsWith(suffix.Trim().ToLower()) with EndsWith(suffix, OrdinalIgnoreCase) and hoist the trimmed suffix out of the per-type predicate. Also fixes a culture-sensitivity smell (ToLower was CurrentCulture). Regression test locks case-insensitive matching. - Q3 EnvironmentVariableBinder: fast-path context.Key when there's no prefix and no formatter (skips the per-property StringBuilder), and collapse the Contains+indexer double lookup to a single IDictionary indexer read. Regression test covers the prefix branch. - Q4 SettingsClassGenerator: cache the generated impl by interface Type in a ConcurrentDictionary instead of re-querying the module by mangled type name on every call. Regression test asserts the same interface returns the cached Type. Build clean on net8.0 + net10.0; 55/55 tests green on net10.0 (was 52). * Refresh handoff and fix-plan (P2 + docs merged, cleanup, Q1–Q4) Bring the running status current: P2 (#18) and the docs tutorials (#20) merged, the #8–#20 workstream branches pruned, and Q1–Q4 quick wins in flight (Q5 was already resolved by B4). Flip the merged checklist items and re-rank next priorities to P3 → P4 → P5, then engine tests (T7 concurrency race still open) and architecture. * Fix M1: namespace-qualify the generated impl type name (Q4 review) Q4's Type-keyed cache exposed a latent collision: GenerateType derived the impl type name from the *simple* interface name (GetNormalizeInterfaceName = Type.Name minus leading I), so two settings interfaces sharing a simple name across namespaces (Foo.ISettings + Bar.ISettings) both mapped to the same module type name. Under the old name-keyed lookup the second silently reused the first's (wrong) type; under the Type-keyed cache the second DefineType now throws and aborts the whole scan. Fix derives the impl name from the namespace-qualified FullName (sanitized), kept deliberately separate from GetNormalizeInterfaceName — that helper also backs SettingsOptions.SectionNameFormatter (the config section name), which must stay simple-name-based. + a regression test generating two same-simple-name interfaces. 56/56 green on net10.0. * Add micro-benchmarks isolating Q1/Q3/Q4 The macro ScanBenchmark can't resolve the quick wins (they're <1% of the IL-emit/populate cost). These isolate each changed hot path so the wins are measurable and trackable: - EnumerateBenchmark (Q1): enumerate a ~2000-entry ISettingsCollection. - EnvBinderBenchmark (Q3): EnvironmentVariableBinder.BindPropertySettings fast path. - GenerateTypeBenchmark (Q4): warm SettingsClassGenerator.GenerateType (cache hit). Grants the benchmark assembly InternalsVisibleTo (Q4 uses the internal generator) and references the Binders project (Q3 uses EnvironmentVariableBinder).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 1 of the code-review fix plan: five self-contained, non-breaking correctness fixes in the settings binding engine, each shipped with a regression test. Verified locally — 88 tests green on
net8.0+net10.0(was 78).Fixes
EnumTypeConverterexisted but was never registered in the converter chain, so an enum property bound from a string fell through toConvert.ChangeTypeand threwInvalidCastException. Registered it ahead of the catch-all converter.DefaultTypeConvertercalledConvert.ChangeTypewith noIFormatProvider. Config values arrive as strings, sodouble/decimalparsed under the ambient culture — e.g."1.5"bound to15on ade-DEhost. Now usesCultureInfo.InvariantCulture.BindingContext.PropertyTypereturned the wrong type. It was set topropertyInfo.DeclaringType(the settings interface) instead of the property's own type — public surface used by customISectionBinderauthors. Also removed two dead, always-false null checks in the constructor.AttributeType = nullto use interface/suffix indication only threwSettingsOptionNonAttributeException, which then hit an NRE building its own message. TheAttributecheck is now guarded by a null check.Resources.TypeIsNotInterfacewas a verbatim string with no$, emitting a literal{typeName}; another message had a stray$before{type.FullName}.Tests
Conversion/DefaultTypeConverterTests— invariant parsing ofdouble/decimalunderde-DE, plus anintbaseline.Conversion/EnumConversionTests— enum-from-string and enum-from-attribute-default.Both files were written to fail against the pre-fix code (confirmed red:
"1.5"→15,"1234.56"→123456, enum threw), then confirmed green after the fixes.Notes